Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DateTime Module functions #465

Closed
wants to merge 2 commits into from
Closed

Add DateTime Module functions #465

wants to merge 2 commits into from

Conversation

lucasteles
Copy link

This module just adds the instance functions of DateTime, this helps to use it in the pipe style and helps the type inference (when we want to use those functions we have to manual type the arguments)

wdyt? should I open an issue for that too?

@gusty
Copy link
Member

gusty commented Aug 2, 2021

I think this needs some discussion, however no need to open an issue, we can also discuss here.

IMHO, the goal of this library shouldn't be to re-write all framework functions in pipe friendly way, and I agree with @dsyme when he states that we should instead embrace dot notation as it's a key feature of F# interoperability.

Having said that, we probably have some functions already in the String module which are or look like a re-write, but they also add the value of being context insensitive.

My feeling is that a DateTime module should bring an additional value, not just a whole rewrite, but if you feel that some functions should be made pipe friendly let's start by compiling a specific set of them, or even better some use cases where operating with the current set of functions become problematic.

PS: One function that I miss and would like to add is something like an efficient implementation of to/of Unix Time.

@wallymathieu
Copy link
Member

There is an RFC to make OO style entity access more palatable in F#.

@dsyme
Copy link
Contributor

dsyme commented Aug 6, 2021

There is an RFC to make OO style entity access more palatable in F#.

The point of the RFC is for cases like xs |> List.map _.Property

That is, even with the RFC the recommendation would always be to use x.Property rather than x |> _.Property unless there is specific reason to do so (which I think there almost never is). The notation x.Property is, frankly, better.

@wallymathieu
Copy link
Member

wallymathieu commented Aug 6, 2021

Indeed @dsyme. I used an overly broad categorisation of the RFC.

@gusty
Copy link
Member

gusty commented Aug 6, 2021

Thanks @dsyme for your clarification

unless there is specific reason to do so (which I think there almost never is).

note that the case being discussed here is mostly forward piping, I guess that's what you consider a valid reason.

@dsyme
Copy link
Contributor

dsyme commented Aug 9, 2021

note that the case being discussed here is mostly forward piping, I guess that's what you consider a valid reason.

Yes for

xs |> List.map _.Property

but I'd imagine almost never for

x |> _.Property

though I guess we'll see

x 
|> some-op
|> _.Property
|> some-op

@gusty
Copy link
Member

gusty commented Aug 9, 2021

Yes, looks like we're on the same page, I agree there's no reason to write x |> _.Property alone.

@gusty
Copy link
Member

gusty commented Jul 31, 2022

Closing this for now for the above reasons, we'll need to compile first a list of functions that have a specific justification.
For instance, the function ToLocalTime shouldn't be added as it depends on the context, we don't want to add functions like that in this module, on the contrary we'll like to make some functions non depending on the context as opposed to its .NET method version (like .ToString vs string).

Feel free to reply with a list of functions and we'll keep discussing and eventually re-open the PR.

@gusty gusty closed this Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants